-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add getAllCompilerAbstracts method to compiler-artefacts.ts
#1
base: contract-verification-plugin-base-reviewing-base
Are you sure you want to change the base?
Add getAllCompilerAbstracts method to compiler-artefacts.ts
#1
Conversation
The current compilerArtefacts plugin's existing methods had two flaws: 1. It wasn't possible to get all Compiler Abstracts available in one go. 2. It was possible to first get all files with `getAllContractsData` and then call each CompilerAbstract one by one with `getCompilerAbstract`, however, in that case the CompilerAbstract was missing the `input` because it wasn't passed to the contructor and saveCompilationPerFileResult. It was only done so for the `solidityUnitTesting` plugin listener. The compiler input is needed for consistent contract verification. While it's possible to generate a compiler input from the contract artefact, via the metadata, it is not always possible to get a match due to known bugs in compiler's AST generation in prev. versions. This results in different bytecode from the original compiler input's output vs the compilation output from the metadata file.
getAllCompilerAbstracts() { | ||
return this.compilersArtefactsPerFile | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added this function
this.compilersArtefactsPerFile[file] = compilerAbstract | ||
} | ||
|
||
onActivation () { | ||
const saveCompilationPerFileResult = (file, source, languageVersion, data, input?) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed this but took the function's contents to saveCompilationResult
const saveCompilationResult = (file, source, languageVersion, data, input?) => { | ||
this.compilersArtefacts.__last = new CompilerAbstract(languageVersion, data, source, input) | ||
this.compilersArtefactsPerFile[file] = new CompilerAbstract(languageVersion, data, source, input) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refactored the contents of listener functions to this single function called saveCompilationResult
. This also contains what the prev. saveCompilationPerFileResult
has
this.compilersArtefactsPerFile[file] = new CompilerAbstract(languageVersion, data, source, input) | ||
this.compilersArtefacts.__last = this.compilersArtefactsPerFile[file] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refactored the contents of listener functions to this single function called saveCompilationResult
. This also contains what the prev. saveCompilationPerFileResult
has.
Basically we always pass the input
to the CompilerAbstract
constructor and then point this one as the __last
artefact. Maybe we might want to create a separate new CompilerAbstract for __last
but this seemed proper to me.e
this.compilersArtefacts.__last = new CompilerAbstract(languageVersion, data, source, input) | ||
saveCompilationPerFileResult(file, source, languageVersion, data) | ||
}) | ||
this.on('solidity', 'compilationFinished', saveCompilationResult) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think saveCompilationResult
can take at most 5 parameters
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it has 5 parameters?
remix-project/libs/remix-core-plugin/src/lib/compiler-artefacts.ts
Lines 36 to 39 in fcc0275
const saveCompilationResult = (file, source, languageVersion, data, input?) => { | |
this.compilersArtefactsPerFile[file] = new CompilerAbstract(languageVersion, data, source, input) | |
this.compilersArtefacts.__last = this.compilersArtefactsPerFile[file] | |
} |
The "compiler input JSON" is needed for consistent contract verification. While it's possible to generate a compiler input from the contract artefact, via the Solidity metadata, it is not always possible to get a match due to known bugs in compiler's AST generation in prev. versions. This results in different bytecode from the original compiler input's output vs the compilation output from the metadata file.
So, to get the compiler input of contracts, I tried using the current compilerArtefacts plugin's existing methods, but they had two flaws:
getAllContractsData
and then call each CompilerAbstract one by one withgetCompilerAbstract
, however, in that case the CompilerAbstract was missing theinput
because it wasn't passed to the contructor and saveCompilationPerFileResult. It was only done so for thesolidityUnitTesting
plugin listener.input
was being saved in__last
but not in other artefacts?input
field from all these listeners? likevyper
,yulp
etc? It seemed yes to me